Skip to content

Conversation

@egparedes
Copy link
Contributor

@egparedes egparedes commented Feb 5, 2026

Renaming and slight refactoring of type definitions used in the gt4py.next toolchain. The main goals are to improve naming clarity and organize better the codebase.

Main changes:

  • Renamed otf.step_types to otf.definitions and extended with more concrete type definitions
  • Renamed toolchain.CompilableProgram to toolchain.ConcreteArtifact to better reflect its generic nature
  • Renamed stage definition classes with "Def" suffix instead of "Definition" for consistency and brevitity
  • Renamed CompilableSource to CompilableProject to emphasize it's a complete project
  • Moved backend-specific functions in otf.arguments to otf.backend
  • Updated all type aliases and references throughout the codebase

Bug fixes for tests
@egparedes egparedes force-pushed the refactor-entry-point branch from 7664c1c to 966a72f Compare February 5, 2026 16:17
@egparedes egparedes marked this pull request as ready for review February 6, 2026 16:17
Copilot AI review requested due to automatic review settings February 6, 2026 16:17
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This is a comprehensive refactoring pull request that renames and reorganizes the entry point types and workflow stages in the GT4Py Next toolchain. The main goals are to improve naming clarity and better organize the codebase structure.

Changes:

  • Renamed CompilableProgram to ConcreteArtifact to better reflect its generic nature
  • Renamed CompilableSource to CompilableProject to emphasize it's a complete project
  • Renamed stage definition classes with "Def" suffix instead of "Definition" for consistency
  • Moved step type protocols from otf.step_types to otf.definitions module
  • Moved jit_to_aot_args function from otf.arguments to backend module
  • Updated all type aliases and references throughout the codebase

Reviewed changes

Copilot reviewed 37 out of 37 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
src/gt4py/next/otf/toolchain.py Core refactoring: CompilableProgramConcreteArtifact, updated type variables
src/gt4py/next/otf/stages.py Removed old CompilableProgram alias, renamed CompilableSourceCompilableProject
src/gt4py/next/otf/definitions.py New home for step protocols, added CompilableProgramDef and related type aliases
src/gt4py/next/ffront/stages.py Renamed all stage classes with "Def" suffix, changed collectionscollections.abc
src/gt4py/next/ffront/decorator.py Updated mixin class name, removed unused Generic parameters, added GTEntryPoint type alias
src/gt4py/next/ffront/field_operator_ast.py Added OperatorNode type alias
src/gt4py/next/backend.py Moved jit_to_aot_args here, updated all type references
src/gt4py/next/typing.py Added GTEntryPoint type alias
Various processor files Updated type references to use new names
Test files Updated to use new type names
docs/user/next/advanced/HackTheToolchain.md Updated examples with new type names (incomplete)
Comments suppressed due to low confidence (3)

src/gt4py/next/typing.py:37

  • The new type alias GTEntryPoint is defined at line 20 but is not exported in the __all__ list. If this type is intended for external use, it should be added to __all__ to make it part of the public API.
    src/gt4py/next/otf/definitions.py:66
  • The docstring at line 66 still references "CompilableSource" which was renamed to "CompilableProject". The docstring should be updated to: "Compile program source code and bindings into a python callable (CompilableProject -> CompiledProgram)."
    tests/next_tests/unit_tests/program_processor_tests/codegens_tests/gtfn_tests/test_gtfn_module.py:82
  • Call to a non-callable of class GTFNTranslationStepFactory.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

class CompilableProgram(Generic[PrgT, ArgT]):
data: PrgT
args: ArgT
class ConcreteArtifact(Generic[DefT, ArgsT]):
Copy link
Contributor Author

@egparedes egparedes Feb 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This name seems the most appropriate for me since this is a super-generic class (this module seems to define only infrastructure to build super-generic pipelines of workflows). The concrete useful types for our compilation toolchain are defined in definitions. A problem I see is that in the actual code this class is used a lot to create instances, which might be confusing. Ideally we should use the more meaningful type aliases instead but I would leave it as it is in this PR and propose to incrementally replace the references with the appropriate concrete definitions in future PRs.

# TODO(ricoh): reconsider name in view of future backends producing standalone compilable ProgramSource code
@dataclasses.dataclass(frozen=True)
class CompilableSource(Generic[SrcL, SettingT, TgtL]):
class CompilableProject(Generic[SrcL, SettingT, TgtL]):
Copy link
Contributor Author

@egparedes egparedes Feb 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that this class contains actual code ready to be compiled, so it doesn't use the Def sufffix, which is used for classes containing different forms of IR or DSL definitions from which actual source code artifacts like this will be generated.

@egparedes egparedes changed the title refactor[next]: Refactor entry point refactor[next]: Rename and refactor toolchain type definitions Feb 6, 2026
@egparedes egparedes requested a review from havogt February 6, 2026 22:01


@dataclasses.dataclass(frozen=True)
class StripArgsAdapter(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The stripping part is not really expressed here, right? It's just a generic transformation from ConcreteArtifact to anything?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The stripping is implemented below in line 65, where ONLY the inp.data is passed on. Or in your terms, it's a generic transformation from ConcreteArtifact to anything with the limitation that only the .data attribute of the ConcreteArtifact can be used in the transformation.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"strip the ConcreteArtifact object of the .args, pass on the rest". Maybe a better name would be "data pass filter"? since only the data is passed on?

Copy link
Contributor

@havogt havogt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be nice if you can add description of how prefix/sufix are used, e.g. Def etc.

Comment on lines +57 to 66
class _CompilableGTEntryPointMixin(Generic[ffront_stages.DSLDefinitionT]):
"""
Mixing used by program and program-like objects.

Contains functionality and configuration options common to all kinds of program-likes.
"""

definition_stage: ProgramLikeDefinitionT
definition_stage: ffront_stages.DSLDefinitionT
backend: Optional[next_backend.Backend]
compilation_options: options.CompilationOptions
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't recognize this class in the first place, so I have to withold judgement on this one. The docstring is a bit on the cryptic side and it's for some reason prefixed with a _.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's purpose seems to be that classes that derive from it can be directly used as compilation stages, without being wrapped. If that's what it does, the new name fits better. Though i am not convinced by the leading underscore, but our coding guidelines are silent about that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This class was recently introduced (see #2368) as a mixin class to support fast-path execution (using a CompiledProgramsPool) of both compiled programs and field operators with variants.

Copy link
Contributor

@DropD DropD left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there is more internal coherence in the naming now, and overall it's an improvement.

The only thing where the naming isn't a strict improvement is where otf.step_types is renamed to otf.definitions and some type aliases added, while the otf.stages, which are definitions as well remain separate.

So here we went from two concretely named separate modules to a genericly named module and an inexplicably separate concretely named one.

@egparedes
Copy link
Contributor Author

The only thing where the naming isn't a strict improvement is where otf.step_types is renamed to otf.definitions and some type aliases added, while the otf.stages, which are definitions as well remain separate.

I agree that definitions is not a very descriptive name for the actual content of the module. The idea is to keep doing small refactoring and renamings incrementally in future PRs, so that the module will aggregate more definitions from other modules. For now it's not a great improvement as you say, but it's planting the seed for future improvements.

@egparedes egparedes merged commit ab93a68 into GridTools:main Feb 9, 2026
23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants